-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix panic when reporting shard diagnostics #2430
Conversation
panic when reporting shards. I also changed the TestSingleServerDiags test to reproduce the error Fixes #2421
@@ -3345,9 +3346,14 @@ func (s *Server) DiagnosticsAsRows() []*influxql.Row { | |||
|
|||
// Shard may not be local to this node. | |||
if sh.store != nil { | |||
shardsRow.Columns = append(shardsRow.Columns, "path") | |||
shardsRow.Values[0] = append(shardsRow.Values[0], sh.store.Path()) | |||
if !hasPath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. If a sh.store
is non-nil, then it must have a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@otoolep but you iterate through each shard. The hasPath variable controls if if the path column has been added before or we need to add it as it's the first time it appears. Otherwise the path column get's appended multiple times unnecessarily
You can ignore the test failure for now, it's a known issue we need to look into. |
OK, thanks @marcosnils -- I'll take a closer look at this diff shortly. |
This code is clearer -- simply append an empty path if the shard is not local. Fixes issue #2430
This code is clearer -- simply append an empty path if the shard is not local. Fixes issue #2430
OK, thanks very much @marcosnils -- I see where you are going. In fact this code has caused multiple issues, so I've actually simplified it at #2431. Your code showed me what was wrong, but I'd like to solve it slightly differently. Please take a look at PR 2431. |
This code is clearer -- simply append an empty path if the shard is not local. Fixes issue #2430
Shard diagnostics wasn't been populated correctly and it made influx panic when reporting shards.
I also changed the TestSingleServerDiags test to reproduce the error
Fixes #2421